Flagging @tustvold who filed the original issue #1938

Thank you @dsgibbons

@tustvold I know this isn't high priority, but just bumping this so I can get some feedback before working on accompanying PRs.

@dsgibbons do you have a usecase for this feature? There is a workaround which is for the user to explicitly call cast on any column types that are not supported

I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog

I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog

The latter

I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog

The latter

Thank you. In that case I will let @tustvold comment as he filed the original ticket and I have no additional context

IIRC the request came from people coming from arrow2 which did something similar. I'm afraid I've lost most context on this and don't have time at the moment to review, but I'm sure one of the other maintainers will be able to advise on a path forward

I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more

Thanks for taking this on @dsgibbons. I may be confused, but it appears that the approach you use for the non-coerced case is to write INT64 with a DATE annotation to the Parquet file. The problem is that the Parquet spec does not allow this (ref). I believe the approach called for in #1938 is to write un-annotated INT64, and rely on the encoded arrow schema to know how to interpret the column. For instance, in arrow/schema/mod.rs, perhaps the logic should be more like

        DataType::Date64 => {
            if coerce_types {
                Type::primitive_type_builder(name, PhysicalType::INT32)
                    .with_logical_type(Some(LogicalType::Date))
                    .with_repetition(repetition)
                    .with_id(id)
                    .build()
            } else {
                Type::primitive_type_builder(name, PhysicalType::INT64)
                    .with_repetition(repetition)
                    .with_id(id)
                    .build()
            }
        },

I don't think the write side is too far off.

On the read side, I think you'll still have to account for the INT32(DATE)->Date64 conversion for the case of coerced data. It seems to me like you've removed the code to handle this case, but again I may be confused.

Thank you for taking the time to look at this @etseidl. I'm still new to the project so I have plenty to learn.

From #1938:

If not coerce_types, write as Int64 and embed logical type in arrow schema only.

I think I interpreted this as the Parquet LogicalType. I hadn't seen that ref before.

I believe the approach called for in #1938 is to write un-annotated INT64, and rely on the encoded arrow schema to know how to interpret the column.

So if we can't embed the fact that the field refers to a date in the Parquet LogicalType, do we provide additional type information during/after reading to interpret INT64 columns as Date64? Is this what was meant by "embed logical type in arrow schema only" from #1938?

I thought that all type information was inferred from the Parquet file. I mistakenly removed the INT32(DATE)->Date64 code, because I didn't think there would be any way to know whether INT32(DATE) was coerced or not. Could you please give an example of how a reader uses an arrow schema to correctly interpret the columns?

On another note, are you OK with the breaking change introduced by: arrow_to_parquet_schema(schema: &Schema, coerce_types: bool)?

Thank you for taking the time to look at this @etseidl. I'm still new to the project so I have plenty to learn.

👋 Welcome! I'm pretty new here too, and still learning as well 😄.

I believe the approach called for in #1938 is to write un-annotated INT64, and rely on the encoded arrow schema to know how to interpret the column.

So if we can't embed the fact that the field refers to a date in the Parquet LogicalType, do we provide additional type information during/after reading to interpret INT64 columns as Date64? Is this what was meant by "embed logical type in arrow schema only" from #1938?

Yes. The Parquet format allows for key-value pairs in the metadata. What the arrow writers will do for certain types that aren't representable in Parquet (most of them time based), is to embed a base64 encoded arrow schema (which is serialized with flatbuffers) in one of these key/value pairs, with the key being ARROW:schema. Take a look at the code here, and trace backwards to see how it's used.

My hope is that the arrow schema will preserve the fact that in arrow it was a Date64 with no further changes (i.e. if coerce==true then do what is currently done, if false then don't scale the value and write as INT64 with no Parquet LogicalType). On the read side, I'm hoping the arrow type will still be Date64, and then you just need to account for the case where the Parquet type is INT64 (the INT32 case is already there since coercing is the current behavior).

Did that make sense? Hopefully someone with more arrow-side experience will correct me if I've messed up some details.

On another note, are you OK with the breaking change introduced by: arrow_to_parquet_schema(schema: &Schema, coerce_types: bool)?

Breaking changes are ok, but with the caveat that they're only allowed in major releases (with the next 54.0.0 release slated for December). IIUC what will happen is once this is PR ready for merge, it will be changed to "Draft" until development on 54.0.0 commences. I'll take another look and see if there's a way to avoid breaking the API. Perhaps new functions that accept the coerce flag, and the existing ones call the new ones passing true. I don't know offhand how many types are impacted, and whether they all currently behave as if coerce is true, though.

Makes sense. I didn't know about ARROW:schema. That helps a lot!

I finally got a chance to update this. I think this is correct now. Fortunately, this PR is a bit shorter now because some of the previous unit tests no longer apply.

I'm not too sure what to do with parquet/src/arrow/arrow_reader/statistics.rs. I made an attempt at this previously, but I'm not sure if what I've done is correct. Any idea what is required here @etseidl?

I think this is an api change so marking it thusly